Skip to content

Conversation

FRSgit
Copy link
Contributor

@FRSgit FRSgit commented Dec 16, 2024

Adds prefer-class-fields rule which turns:

class Foo {
  constructor() {
    this.foo = 'foo';
  }
}

into:

class Foo {
  foo = 'foo';
}

Fixes #314


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


@fisker
Copy link
Collaborator

fisker commented Dec 17, 2024

What should we do if the assignment is unreachable?

			class Foo {
				constructor(x) {
					if (x) {return}
					this.foo = 'foo';
				}
			}

@fregante
Copy link
Collaborator

What should we do if the assignment is unreachable?

It's dynamically set, so it cannot be statically defined. It should not be reported by this rule.

@FRSgit
Copy link
Contributor Author

FRSgit commented Dec 20, 2024

Hey @fregante!
Thanks for your input about the linting - is there a ruleset somewhere which I could use? The lint task in this repo is not doing this much (at least for me) and it removes some of your changes 😄 E.g. it seems that the part /** @type {import('eslint').Rule.RuleModule} */ is required by the lint task - after your change it was failing with:
image
So, I've reverted this one change.

Or maybe we can just agree that once the work in this PR is done you'll maybe just run your linter on it once?

@FRSgit
Copy link
Contributor Author

FRSgit commented Dec 22, 2024

Handled stopping of the static analysis on unsupported case: 3376845

@FRSgit
Copy link
Contributor Author

FRSgit commented Dec 26, 2024

Okay, I fixed all test/linting errors connected with my rule - should I fix other as well? They seems to be connected with some default options missing in already existing rules?
cc: @sindresorhus @fregante @fisker

@FRSgit
Copy link
Contributor Author

FRSgit commented Jan 16, 2025

Hey! I've merged last changes from main to the branch of this PR - now it's ready to be reviewed again

@sindresorhus
Copy link
Owner

Change the test file extension from .mjs to .js.

@FRSgit
Copy link
Contributor Author

FRSgit commented Jan 21, 2025

@sindresorhus done: 8358a43

I'm not sure yet why the tests are failing - locally they're working just fine.

@FRSgit FRSgit requested a review from sindresorhus January 29, 2025 00:30
@FRSgit
Copy link
Contributor Author

FRSgit commented Jan 29, 2025

Okay, got it working and all of the requested changes are applied.

BTW it's weird, but for me the npm run fix:eslint-docs script worked only on node@22.
Maybe it would be good to add an .nvmrc file to the root of the repo with the node version that should be used when running npm scripts?

@FRSgit FRSgit requested a review from fisker January 31, 2025 09:22
@FRSgit
Copy link
Contributor Author

FRSgit commented Feb 15, 2025

ready to be re-reviewed @fisker

Comment on lines 117 to 120
const validConstructorProperties
= firstInvalidProperty === -1
? constructorBody
: constructorBody.slice(0, firstInvalidProperty);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already use a loop, we can use the index directly, no need slice here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, changed implementation to set index variable for initial loop iteration: 93010be

@sindresorhus
Copy link
Owner

@FRSgit Bump :)

@fisker
Copy link
Collaborator

fisker commented Jun 12, 2025

Fixed the fix logic for some edge cases, especially

class Foo {
constructor() {
	this.bar = 1;
}
static}

@sindresorhus sindresorhus merged commit 4c82dc1 into sindresorhus:main Jun 14, 2025
19 checks passed
@FRSgit FRSgit deleted the feat/add-prefer-class-fields branch June 15, 2025 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: prefer-class-fields
4 participants